Skip to content

Tidy up the package.json#35

Merged
DanielJDufour merged 3 commits intostac-utils:mainfrom
rowanwins:package-fixes
Jun 9, 2022
Merged

Tidy up the package.json#35
DanielJDufour merged 3 commits intostac-utils:mainfrom
rowanwins:package-fixes

Conversation

@rowanwins
Copy link
Copy Markdown
Contributor

This PR fixes up some of the pointers in the package.json.
Currently the stac-layer.min.js is 1.3MB. I'm pretty sure it's bundling all the dependencies such as leaflet.js etc in a way that means they might not being tree-shaken.
This PR should mostly resolve the issues in this repo, but I'm going to open some similar PR's upstream in georaster packages to try and fix some similar issues (eg GeoTIFF/georaster#65).

@DanielJDufour
Copy link
Copy Markdown
Member

I can't remember why main was originally set to "dist/stac-layer.min.js". I just know I've similarly set main to a build when there was some issue with a dependency using a more recent JS syntax like ??= or something like that. Sometimes implementers of stac-layer might be using something like create-react-app that doesn't support all the most recent cool JS syntax. However, I think Google Chrome now supports ??=, so this probably isn't going to be a common problem. I think you can keep "main": "src/index.js", in the PR, but just wanted to share with you the original reasoning in case this comes up in the future. (And also because I'm likely to forget my original reasoning unless I write it down!)

Could you also set jsdelivr to "dist/stac-layer.min.js" (which you may already know is what ObservableHQ uses for their default AMD loading)? This isn't required for merging because it wasn't there originally, but it would be a nice to have.

I think you mentioned this in another thread, but the bulk of the build is probably coming from https://www.npmjs.com/package/proj4js-definitions. Just wanted to mention this here in case anyone else is interested and reading this.

I intentionally specified all the files in "files: [...]" instead of using files: "["src", "dist"]" for a couple reasons: I use pkg-ok in my publishing workflow and I know it works with files. (I haven't tried with directories, so it's "probably" not a problem for this reason). The most important reason, is that I want to be intentional about which files are included and make sure I don't include any other files in the build. I usually have a lot of development files where I'm trying out new things or want to save some code to reference later. I save them in .bak files so they are ignored by git: https://github.com/stac-utils/stac-layer/blob/main/.gitignore#L107
Screen Shot 2022-04-23 at 9 45 42 PM
I also generally directly specify the files, because I don't want to accidentally include any personal information/files in a npm package. If we don't specify the files, I'll probably have to add a manual step of running npm pack and manually inspecting the list of which files are going to be published. Long story short, if it's not too much an inconvenience, I'd prefer keeping files the way it is. I only have a medium-level of preference knowing it's mostly about my own weird setup and personal preference, so if you have a strong opinion on it, I'm happy to accommodate and accept the change :-)

@m-mohr
Copy link
Copy Markdown
Collaborator

m-mohr commented Apr 24, 2022

Yeah, I think we put the .min file in there as we had issues with vue-cli/webpack bundling the new syntax of the Nullish coalescing operator etc. I'm not sure whether this has been solved yet. We should check this so that we can ensure nothing breaks, because gaining the mentioned benefits would be great.

@m-mohr
Copy link
Copy Markdown
Collaborator

m-mohr commented Jun 5, 2022

Actually, I think we should not care whether downstream there's an issue. If there's one, we need to fix it downstream. It's really a bad idea to fix something here that in reality should be fixed somewhere else. Let's merge this and then we'll figure things out in STAC Browser once released. This should be released as 0.11.0 and not as 0.10.2 ideally.

Edit: Did some tests and this seems to work after upgrading some dependencies in STAC Browser.

@m-mohr m-mohr requested a review from DanielJDufour June 5, 2022 16:49
@m-mohr m-mohr mentioned this pull request Jun 5, 2022
@m-mohr m-mohr added this to the 0.11.0 milestone Jun 5, 2022
@DanielJDufour DanielJDufour merged commit 4900d11 into stac-utils:main Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants